Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ecdsa_to_eth_address() and remove eth_compatibility crate #1233

Merged
merged 22 commits into from
May 13, 2022

Conversation

agryaznov
Copy link
Contributor

@agryaznov agryaznov commented Apr 24, 2022

Ported the following function from eth_compatibility crate to ink_env:

pub fn ecdsa_to_eth_address(pubkey: &[u8; 33], output: &mut [u8; 20]) -> Result<()>

eth_compatibility crate removed as having no reason to exist anymore

@athei
Copy link
Contributor

athei commented Apr 24, 2022

The function shouldn't be removed. It should be replaced with a call to the new seal function.

@agryaznov
Copy link
Contributor Author

agryaznov commented Apr 24, 2022

I was just thinking on removal the whole ink_eth_compatibility crate and adding this seal function just like any other seal function into ink_env and ink_lang

@agryaznov agryaznov changed the title Remove ink_eth_compatibility::to_eth_address() and libsecp256k1 dep Remove eth_compatibility crate and implement ecdsa_to_eth_address() Apr 30, 2022
crates/lang/src/env_access.rs Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented May 2, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.3.0-824c2f6 and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.04 K
adder 2.13 K
contract-introspection 2.37 K
contract-terminate 0.94 K 275_000
contract-transfer 8.13 K 75_000
data-structures 1.73 K
delegate-calls 2.96 K 76_276
delegator 6.38 K 232_281
dns 8.84 K 225_000
erc1155 17.34 K 450_000
erc20 8.49 K 225_000
erc721 11.81 K 600_000
flipper 1.31 K 75_000
forward-calls 2.88 K 151_427
incrementer 1.21 K
multisig 25.33 K 470_285
rand-extension 3.92 K 75_000
seal-code-hash 1.58 K
set-code-hash 1.57 K 150_000
subber 2.15 K
trait-erc20 8.77 K 225_000
trait-flipper 1.00 K 75_000
trait-incrementer 1.19 K 150_000
updated-incrementer 9.69 K
upgradeable-flipper 1.55 K

Link to the run | Last update: Thu May 12 14:19:45 CEST 2022

@codecov-commenter
Copy link

codecov-commenter commented May 3, 2022

Codecov Report

Merging #1233 (a0e429a) into master (2051044) will increase coverage by 17.09%.
The diff coverage is 61.11%.

❗ Current head a0e429a differs from pull request most recent head 388b136. Consider uploading reports for the commit 388b136 to get more accurate results

@@             Coverage Diff             @@
##           master    #1233       +/-   ##
===========================================
+ Coverage   61.64%   78.74%   +17.09%     
===========================================
  Files         228      228               
  Lines        8654     8675       +21     
===========================================
+ Hits         5335     6831     +1496     
+ Misses       3319     1844     -1475     
Impacted Files Coverage Δ
crates/env/src/backend.rs 83.33% <ø> (ø)
crates/lang/src/env_access.rs 12.00% <ø> (ø)
crates/env/src/api.rs 32.43% <50.00%> (+10.37%) ⬆️
crates/env/src/engine/off_chain/impls.rs 43.67% <66.66%> (+9.72%) ⬆️
...ates/storage/src/collections/hashmap/fuzz_tests.rs 95.74% <0.00%> (-4.26%) ⬇️
crates/allocator/src/bump.rs 0.00% <0.00%> (ø)
crates/storage/src/traits/impls/prims.rs 96.92% <0.00%> (+1.53%) ⬆️
crates/lang/ir/src/ir/attrs.rs 82.27% <0.00%> (+3.87%) ⬆️
crates/metadata/src/layout/mod.rs 77.01% <0.00%> (+4.59%) ⬆️
crates/lang/ir/src/ir/trait_def/item/mod.rs 90.16% <0.00%> (+4.91%) ⬆️
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2051044...388b136. Read the comment docs.

@agryaznov agryaznov changed the title Remove eth_compatibility crate and implement ecdsa_to_eth_address() Implement ecdsa_to_eth_address() and ecdsa_to_default_account_id(), remove eth_compatibility crate May 11, 2022
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
agryaznov and others added 2 commits May 12, 2022 10:13
Co-authored-by: Michael Müller <mich@elmueller.net>
@agryaznov
Copy link
Contributor Author

After further discussion it was finally decided to remove ecdsa_to_default_account_id()

@agryaznov agryaznov changed the title Implement ecdsa_to_eth_address() and ecdsa_to_default_account_id(), remove eth_compatibility crate Implement ecdsa_to_eth_address() and remove eth_compatibility crate May 12, 2022
@agryaznov agryaznov requested a review from cmichi May 12, 2022 15:52
@cmichi cmichi merged commit 785a59e into master May 13, 2022
@cmichi cmichi deleted the ag-libsecp256k1-remove branch May 13, 2022 07:46
agryaznov added a commit that referenced this pull request Jun 6, 2022
agryaznov added a commit that referenced this pull request Jun 22, 2022
* Fix links in release notes (#1277)

* Revert "Optimise deny_payment. Use eerywhere semantic of deny. (#1267)"

This reverts commit 1bfccc7.

* Revert backward-incompatible piece of #1224: dependency on `[seal1] seal_set_storage`

* Revert backward-incompatible piece of #1233: removal of eth_compatibility crate

* bump crate versions + update RELEASES.md

* Mapping::insert_return_size is back, having now both `seal1` and `seal0` seal_set_storage versions used

* set_storage_silent -> set_storage_compat renaming

* spell fix

* Apply suggestions from code review

Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Update crates/env/src/backend.rs

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* doc comments enhanced

* `Mapping::insert()` to use backwards compatible seal fn

* unreleased changes planned for 4.x removed from 3.x

* Add more details to the release notes

* fix catched issue with changed api function signature

* fix storage trait dependent func

* Apply new versions naming policy: step1. Old versions to keep their names.

* Apply new versions naming policy: step2. Add `deprecated` attr and `# Compatibility` docs section

* Apply suggestions from code review

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* fixes after next review round

* more fixes after the review round

* fmt

* spellcheck config fix

* Small wording fixes

Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <hernando@hcastano.com>
@ascjones ascjones mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove libsecp256k1 dependency of ink_eth_compatibility
5 participants